-
-
Notifications
You must be signed in to change notification settings - Fork 39.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] allow I2C_[SDA|SCL]_PAL_MODE to be (p)redefined #19286
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure why these are being set in chibios_config.h
in the first place; they should probably be moved to i2c_master.c
: https://github.com/qmk/qmk_firmware/blob/master/platforms/chibios/drivers/i2c_master.c#L33
note that these are in a "#if defined(MCU_RP)" block! |
c7ba2eb
to
fa70eba
Compare
... added to the commit message that this targets RP2040 |
But now these defaults are fragmented across multiple files. chibios_config.h, as far as I understand it, is meant to be a translation layer (for example the GD32 and WB32 stuff), so it should not contain anything that might need to be overridden. |
83cfd60
to
3b3e09d
Compare
shuffled things around - like this then? |
But should the supposedly generic Maybe |
@JohSchneider out of curiosity what are the changes in the modes did you had to do in order to get i2c working with you device? Maybe the defaults could need a change. |
@zvecr requested to move the mcu specific logic/data out of |
There is already plenty of MCU-specific code in these drivers. It's difficult to avoid when we have to support all these different families, though ChibiOS does help out a lot. I guess the issue is that the RP2040's default I2C pin configuration is more specific than simply "open drain", therefore we can't just define it for |
The problem with RP2040 is that it does not implement the open drain mode for GPIOs at all — it is possible to emulate it by setting the pin to output 0 and manipulating the output enable bit appropriately, but the existing GPIO code does not try to implement that workaround, therefore BTW, a similar problem could appear with WB32 (the code in Another difference between placing the define in |
To answer my own question, in #17902 you wrote:
But the default pad drive strength is already 4ma both in ChibiOS and as the default value after boot-up in the rp2040. So (in theory) that change should have no effect. |
empirical practice lead me to the "remove PAD_DRIVE4 == works" conclusion s-: |
If the drive strength is not specified, it probably ends up being 2, not 4: #define PAL_RP_PAD_DRIVE2 (0U << (24 + 4))
#define PAL_RP_PAD_DRIVE4 (1U << (24 + 4))
#define PAL_RP_PAD_DRIVE8 (2U << (24 + 4))
#define PAL_RP_PAD_DRIVE12 (3U << (24 + 4)) |
Oh well, you are completely right. I forgot that we override the whole register with the new values. |
Thank you for your contribution! |
3b3e09d
to
0c829f3
Compare
Thank you for your contribution! |
de-fragmenting/separating the common presets from the user-overridables Signed-off-by: Johannes Schneider <[email protected]>
0c829f3
to
31e2fcd
Compare
un-stale: rebasing onto current develop any chance to get a consensus - and possibly approval? :-) |
Does the current code ( |
Thank you for your contribution! |
Thank you for your contribution! |
some i2c devices might not work properly with the pre-set default values on the i2c pads
allow users to redefine/reconfigure the i2c pads on chibios-based builds by wrapping the define in a 'ifndef'
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist